-
Notifications
You must be signed in to change notification settings - Fork 43
feat: headless app and api for testing js-waku in browser #2371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
3b260f8
to
f295a35
Compare
8c53464
to
cffbef3
Compare
size-limit report 📦
|
1d822b1
to
365833c
Compare
Ready for review? Issue description would be nice:D |
ping @adklempner |
@danisharora099 @weboko updated! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new browser testing infrastructure that replaces the old testing structure with a headless web app and server API endpoints that align with the nwaku REST API. Key changes include:
- Implementation of a new Express server to run a headless browser with Playwright.
- Introduction of multiple API endpoints (info, push, admin, etc.) to interact with the Waku node.
- Updates to configuration files, Dockerfile, and CI workflows to support the new structure.
Reviewed Changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
packages/browser-tests/src/server.ts.new | New server implementation to serve the headless app with API endpoints and browser initialization. |
packages/browser-tests/src/routes/* | New API routes for legacy and REST API compatible pushes and node management. |
packages/browser-tests/src/queue/index.ts | Message queue implementation for storing and retrieving messages. |
packages/browser-tests/src/browser/index.ts | Browser initialization and page management for the headless tests. |
packages/browser-tests/src/api/* | New shared API functions wrapping Waku SDK functionality for operations like push, node creation, and subscriptions. |
packages/browser-tests/playwright.config.ts, package.json, Dockerfile, CI workflows | Config and build updates to integrate the headless app and testing infrastructure. |
packages/browser-tests/README.md | Updated documentation describing the new architecture and usage examples. |
Comments suppressed due to low confidence (1)
Dockerfile:33
- Ensure consistency in naming; the Dockerfile refers to a 'headless' directory while other scripts and CI workflows refer to 'headless-tests'. Align these names to prevent confusion across environments.
COPY headless/package.json ./headless/
export async function pushMessage( | ||
waku: LightNode, | ||
contentTopic: string, | ||
payload?: Uint8Array<ArrayBuffer> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider revising the type annotation for payload; using 'Uint8Array' without a generic parameter is standard and may avoid potential type mismatches.
Copilot uses AI. Check for mistakes.
.eslintrc.json
Outdated
"build", | ||
"coverage", | ||
"proto", | ||
"**/webpack.config.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we suppress it in tsconfig
of a particular projects?
.github/workflows/ci.yml
Outdated
repository: waku-org/js-waku | ||
- uses: actions/setup-node@v3 | ||
with: | ||
node-version: ${{ env.NODE_JS }} | ||
- uses: ./.github/actions/npm | ||
- name: Install headless-tests dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since headless-tests
is part of packages in root package.json
- this step is not needed as - uses: ./.github/actions/npm
should trigger installation
.github/workflows/ci.yml
Outdated
repository: waku-org/js-waku | ||
- uses: actions/setup-node@v3 | ||
with: | ||
node-version: ${{ env.NODE_JS }} | ||
- uses: ./.github/actions/npm | ||
- name: Install headless-tests dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't Install headless-tests dependencies
needed only for browser-test
step?
.github/workflows/playwright.yml
Outdated
@@ -29,6 +29,12 @@ jobs: | |||
|
|||
- uses: ./.github/actions/npm | |||
|
|||
- name: Install headless-tests dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't seem to be needed due to ./.github/actions/npm
called prior
package.json
Outdated
@@ -17,6 +17,7 @@ | |||
"packages/rln", | |||
"packages/tests", | |||
"packages/browser-tests", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if browser-tests
depends on headless-tests
- then they should be defined in different order in the array
also, about the naming, maybe it is better to rename:
browser-tests
->browser-container
;headless-tests
->browser-tests
;
That way we define that one is container and another is test cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's do this separate because it will make review terrible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adklempner why? it seems that just lines needs to be swapped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adklempner why? it seems that just lines needs to be swapped
I meant the renaming, you are correct about the order
File |
packages/browser-tests/package.json
Outdated
"start": "npm run start:server", | ||
"start:serve": "cd ../headless-tests && npx serve -p 8080 --no-port-switching . -s", | ||
"test": "npx playwright test", | ||
"build:headless": "cd ../headless-tests && npm run build", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be able instead of cd ../headless-tests && npm run build
to do something like npm run build --workspace=headless-tests
in this and similar places, removing cd ../headless-tests
altogether
let me know if it works, I am not 100% sure
name: "firefox", | ||
use: { ...devices["Desktop Firefox"] } | ||
}, | ||
// { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed anymore? if so, let's remove
b3216da
to
db878fc
Compare
.eslintrc.json
Outdated
"build", | ||
"coverage", | ||
"proto", | ||
"**/webpack.config.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it doesn't seem to be needed anymore as webpack.config.js
looks good now from lint perspective
Dockerfile
Outdated
COPY packages/headless-tests/package.json ./packages/headless-tests/ | ||
|
||
# Install dependencies | ||
RUN npm install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this npm install
should be enough and 2 following are just verbose
contentTopic: string, | ||
payload?: Uint8Array<ArrayBuffer> | ||
): Promise<SDKProtocolResult> { | ||
// encoder and decoder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this comment doesn't seem to be needed anymore
`Creating decoder for content topic ${contentTopic} with clusterId=${clusterId}, shard=${shard}` | ||
); | ||
|
||
// Construct the pubsub topic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this file and others a lot of comments like this are not needed and just create noise
I understand that these are most likely added by AI,
but if we need a comment - let's better try to write a self-documenting code - https://jawlon.medium.com/clean-code-writing-readable-and-maintainable-code-946a5b28a1d9
🙏 @waku-org/js-waku
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed most of the comments, I left some that I thought were useful
|
||
const router = Router(); | ||
|
||
// Legacy push message endpoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it legacy push
?
packages/browser-tests/src/server.ts
Outdated
} | ||
}) as express.RequestHandler); | ||
|
||
// NEW ENDPOINT: Create a Waku node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it duplicate of those in admin.ts
?
packages/browser-tests/src/server.ts
Outdated
|
||
// NEW ENDPOINT: Create a Waku node | ||
app.head("/admin/v1/create-node", (_req: Request, res: Response) => { | ||
res.status(200).end(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doens't seems to do anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the comment but the purpose of HEAD
is to tell a client "does this endpoint exist? can I access it?" In this case there's a corresponding POST
endpoint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it is not used I would remove it and just keep POST
@@ -0,0 +1 @@ | |||
[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this file for?
packages/headless-tests/package.json
Outdated
"@babel/preset-env": "^7.24.0", | ||
"@babel/preset-typescript": "^7.23.3", | ||
"babel-loader": "^9.1.3", | ||
"gh-pages": "^4.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it needed?
3d871e3
to
0f6fc55
Compare
}; | ||
}; | ||
lightPush: { | ||
// eslint-disable-next-line no-unused-vars |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it doesn't seem to be needed, there are no unused vars
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, but had to switch it to a .d.ts
file
} | ||
} else { | ||
// Default to cluster ID 42, shard 0 as per the tests | ||
nodeOptions.networkConfig = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is something to be discussed and agreed with DST team I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but anyway network config must be provided for this to work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made network config required, throws error if not defined
} | ||
|
||
// Cluster 42 | ||
const peers = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is repeated across .spec files, move it to some .env file or const.ts
@@ -0,0 +1,13031 @@ | |||
{ | |||
"name": "headless", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is submodule it shouldn't have it's own package-lock
run-ci.sh
Outdated
@@ -0,0 +1,39 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it used anywhere? I can't find
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the comments — it looks much better now!
There are still a few remaining ones to resolve before we can merge.
Approving preliminary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a headless web app and server API for testing the js‑waku node in a browser environment while aligning the endpoints with the nwaku REST API. Key changes include the implementation of new admin routes for node actions, the addition of shared API functions to interact with the Waku node, and significant updates to build, Docker, and CI configurations to support the containerized headless testing infrastructure.
Reviewed Changes
Copilot reviewed 46 out of 46 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
packages/browser-tests/src/routes/admin.ts | New admin endpoints for creating, starting, stopping nodes and dialing peers |
packages/browser-tests/src/queue/index.ts | Message queue utilities for browser tests |
packages/browser-tests/src/build-example.js | Removal of the legacy build example script |
packages/browser-tests/src/browser/index.ts | Browser management functions using Playwright |
packages/browser-tests/src/api/* | Shared API functions for interacting with the Waku node |
packages/browser-tests/playwright.config.ts | Configuration update to dynamically load dotenv-flow and simplify browser testing |
packages/browser-tests/package.json & Dockerfile | Updates to scripts, dependencies, and container build/run configurations |
Comments suppressed due to low confidence (2)
packages/browser-tests/src/api/shared.ts:250
- Consider handling subscription failures more explicitly instead of returning a no-op unsubscribe function. This change will allow client code to more clearly distinguish between successful subscriptions and fallback cases.
return { unsubscribe: async () => { console.log("No-op unsubscribe from failed subscription"); } } as unknown as SubscribeResult;
Dockerfile:45
- Verify that the workspace flag '-w' is required and functions as expected in the Docker container environment; if the working directory is already set appropriately, this flag may be redundant and could be removed for clarity.
CMD ["npm", "run", "start:server", "-w", "packages/browser-tests"]
f141f33
to
f6dc1e2
Compare
f6dc1e2
to
2f90a45
Compare
Problem / Description
The current browser test framework has limitations in its structure and integration capabilities. We need a more streamlined approach that better aligns with the nwaku REST API, allowing for seamless testing across different environments including Docker containers used by the Distributed Systems Testing team.
Solution
Notes
Checklist